Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce entity animations #806

Closed
wants to merge 7 commits into from

Conversation

RoyalMCPE
Copy link
Contributor

The documentation probably needs improvement. Also tie-in pr for the wiki

Tests

p.World().PlayAnimation(p, animation.New("animation.player.sit").WithController("controller.animation.player.sit").WithState("sit").WithStopCondition("query.is_jumping"))

Test resource pack: EntityAnimationTest.zip

server/world/world.go Show resolved Hide resolved
Copy link
Member

@TwistedAsylumMC TwistedAsylumMC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few things mostly with docs. Is State an appropriate name for the field & methods considering it's actually the next state?

@@ -915,9 +917,12 @@ func (s *Session) ViewEntityState(e world.Entity) {
}

// ViewEntityAnimation ...
func (s *Session) ViewEntityAnimation(e world.Entity, animationName string) {
func (s *Session) ViewEntityAnimation(e world.Entity, animation animation.Animation) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just rename this variable to a to avoid conflicting with the package, same thing in the interface yupe in viewer.go

Comment on lines +13 to +18
return Animation{
name: animationName,
state: "",
controller: "",
stopCondition: "",
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be Animation{name: name}

Comment on lines +3 to +4
// Animation represents an animation & controller that may be attached to an entity.
// Animations and controllers must be defined in a resource pack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this would sound better

// Animation represents an animation that may be played on an entity from an active resource pack on
// the client.

Comment on lines +26 to +27
// WithController sets the controller with the specified state.
// The controller must be added in a resource pack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// WithController returns a copy of the Animation with the provided animation controller. The
// controller must be defined in a resource pack for it to work.

Comment on lines +33 to +34
// Controller returns the name of the controller being used. Controller returns an empty string if
// no controller was previously set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it's necessary to mention the empty string here, also purely just for OCD reasons could you move Controller(), State() and StopCondition() methods to be before their respective With...() methods?

return a.controller
}

// WithState sets the state to transition to as defined in the controller.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// WithState returns a copy of the Animation with the provided next state.

Comment on lines +45 to +46
// State returns the current state being played. State returns an empty string if
// no controller was previously set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing, no point mentioning empty string. This is also incorrect since it's not the state that is currently being played. Something like this may be better:

// State returns the name of the next state in the animation controller.

return a.state
}

// WithStopCondition takes the molang expression and stops the animation if the query passes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// WithStopCondition returns a copy of the Animation with the provided stop condition. This condition is
// usually a Molang query which is constantly compared against until the animation has stopped.

Comment on lines +57 to +58
// StopCondition returns the stop condition. StopCondition returns an empty string if
// no molang expression was set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing, no point mentioning empty string

Comment on lines +648 to +650
// AnimateEntity will start an animation for the specified entity. Viewers that are viewing the entity will be
// played the animation.
func (w *World) AnimateEntity(e Entity, animation animation.Animation) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PlayEntityAnimation might be a better name to fit with PlaySound etc.

@TwistedAsylumMC
Copy link
Member

Closing in favour of #940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants